Properly constrain the basic_json conversion operator #2825

ldionne · 2021-06-17T17:32:15Z

This was reported as a libc++ bug in http://llvm.org/PR48507, but it's a bug in this library, not in libc++. See Richard Smith's explanation in http://llvm.org/PR48507.

ldionne · 2021-06-17T17:33:52Z

I'm not sure what standards you support. If some of the type traits (e.g. std::conjunction) are not available on all the compilers you support, it's easy to fix that by implementing our own. Just let me know.

I'm now going to close http://llvm.org/PR48507 because it's not a libc++ bug.

alexezeder

Just two comments to compare with my PR. The appearance of a new PR made me update mine. 😉

alexezeder · 2021-06-17T19:06:00Z

include/nlohmann/detail/meta/detected.hpp

+template<template<class...> class Op, class... Args>
+struct is_detected_lazy : is_detected<Op, Args...> { };


IMHO, That's done way better than in my PR. I like the idea of making the number of additional instantiations as small as possible, and this allows to eliminate unnecessary instantiations. 👍

Just two comments to compare with my PR. The appearance of a new PR made me update mine. 😉

Sorry, I didn't realize there was already a PR for this.

alexezeder · 2021-06-17T19:16:31Z

include/nlohmann/json.hpp

+                    std::conjunction<
+                        std::negation<std::is_pointer<ValueType>>,
+                        std::negation<std::is_same<ValueType, detail::json_ref<basic_json>>>,
+                        std::negation<std::is_same<ValueType, typename string_t::value_type>>,
+                        std::negation<detail::is_basic_json<ValueType>>,
+                        std::negation<std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>>,
+
 #if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))
-                   && !std::is_same<ValueType, typename std::string_view>::value
+                        std::negation<std::is_same<ValueType, std::string_view>>,
 #endif
-                   && detail::is_detected<detail::get_template_function, const basic_json_t&, ValueType>::value
-                   , int >::type = 0 >
+                        detail::is_detected_lazy<detail::get_template_function, const basic_json_t&, ValueType>
+                    >::value, int >::type = 0 >


IMHO, there is no much sense to use templated/typed/parametrized logic operations for all operands. For the same reason as above. Of course, it can backfire in the future, but it wouldn't hurt too much since these operands can be replaced pretty quickly.

You could technically do something like:

std::conjunction< std::bool_constant< !std::is_pointer<ValueType>::value && !std::is_same<ValueType, detail::json_ref<basic_json>>::value && !std::is_same<ValueType, typename string_t::value_type>::value && !detail::is_basic_json<ValueType>::value && !std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value && // ... >, detail::is_detected_lazy<detail::get_template_function, const basic_json_t&, ValueType> >

But I don't think it provides a big benefit, and it's less consistent.

Yeah, actually, it's

json/include/nlohmann/json.hpp

Lines 3345 to 3357 in ae9cfb1

detail::conjunction <

std::integral_constant < bool,

!std::is_pointer<ValueType>::value&&

!std::is_same<ValueType, detail::json_ref<basic_json>>::value&&

!std::is_same<ValueType, typename string_t::value_type>::value&&

!detail::is_basic_json<ValueType>::value

&& !std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value

#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) || (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))

&& !std::is_same<ValueType, typename std::string_view>::value

#endif

>,

detail::is_detected<detail::get_template_function, const basic_json_t&, ValueType >>::value

, int >::type = 0 >

You are right about consistency for sure. Does it give a big benefit - I don't know, I'm just hoping that it helps. But looking more at this library, probably a few more instantiations wouldn't be noticeable.

coveralls · 2021-07-10T11:11:49Z

Coverage remained the same at 100.0% when pulling b0e5965 on ldionne:ldionne-lazy into 03270ef on nlohmann:develop.

nlohmann

Can you please update this branch to the latest develop branch?

test/src/unit-bug2491.cpp

include/nlohmann/detail/meta/type_traits.hpp

ldionne · 2021-07-19T13:57:59Z

Rebased onto develop and renamed test as requested.

test/src/unit-regression3.cpp

nlohmann

The test for the amalgamation (https://github.com/nlohmann/json/runs/3104916288?check_suite_focus=true) failed. Can you please run make amalgamate - this should re-indent and regenerate the single header.

ldionne · 2021-07-19T15:04:55Z

The test for the amalgamation (https://github.com/nlohmann/json/runs/3104916288?check_suite_focus=true) failed. Can you please run make amalgamate - this should re-indent and regenerate the single header.

It looks like there's nothing to be done on my system for amalgamate:

% make amalgamate
make: Nothing to be done for `amalgamate'.

nlohmann · 2021-07-19T15:07:12Z

It looks like there's nothing to be done on my system for amalgamate:
% make amalgamate
make: Nothing to be done for `amalgamate'.

That is strange. Can you try to delete file single_include/nlohmann/json.hpp and re-run make amalgamate?

Fixes #2491

ldionne · 2021-07-19T15:18:00Z

It looks like there's nothing to be done on my system for amalgamate:
% make amalgamate
make: Nothing to be done for `amalgamate'.
That is strange. Can you try to delete file single_include/nlohmann/json.hpp and re-run make amalgamate?

Now that seems to make a difference, thanks.

nlohmann · 2021-07-19T15:18:39Z

Thank you for your patience!

nlohmann · 2021-07-22T07:38:08Z

The remaining issue in the CI has been fixed in the develop branch, so technically this PR is ready to be merged.

As I am no expert in template magic, I do no really understand the fix. @gregmarr @alexezeder can either of you have a quick look whether the changes are OK to you?

alexezeder · 2021-07-22T09:18:36Z

@alexezeder can either of you have a quick look whether the changes are OK to you?

I cannot call myself an expert in template magic either, but since I understand what causes the errors here - the changes are OK to me. I've already reviewed this PR, and I agree with the @ldionne that it's not worth changing logic operations for all operands for consistency. Also, as @ldionne mentioned, it doesn't provide a big benefit.

nlohmann

Looks good to me.

nlohmann · 2021-07-22T10:25:03Z

Thanks!

ldionne requested a review from nlohmann as a code owner June 17, 2021 17:32

ldionne mentioned this pull request Jun 17, 2021

Compilation error when using NLOHMANN_JSON_SERIALIZE_ENUM ordered_json on libc++ #2491

Closed

3 tasks

alexezeder reviewed Jun 17, 2021

View reviewed changes

nlohmann requested changes Jul 15, 2021

View reviewed changes

test/src/unit-bug2491.cpp Outdated Show resolved Hide resolved

include/nlohmann/detail/meta/type_traits.hpp Outdated Show resolved Hide resolved

nlohmann requested changes Jul 19, 2021

View reviewed changes

test/src/unit-regression3.cpp Outdated Show resolved Hide resolved

nlohmann requested changes Jul 19, 2021

View reviewed changes

Properly constrain the basic_json conversion operator

b0e5965

Fixes #2491

nlohmann added state: please discuss please discuss the issue or vote for your favorite option review needed It would be great if someone could review the proposed changes. and removed state: please discuss please discuss the issue or vote for your favorite option labels Jul 22, 2021

nlohmann self-assigned this Jul 22, 2021

nlohmann added release item: 🐛 bug fix and removed review needed It would be great if someone could review the proposed changes. labels Jul 22, 2021

nlohmann added this to the Release 3.9.2 milestone Jul 22, 2021

nlohmann approved these changes Jul 22, 2021

View reviewed changes

nlohmann merged commit a563338 into nlohmann:develop Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly constrain the basic_json conversion operator #2825

Properly constrain the basic_json conversion operator #2825

ldionne commented Jun 17, 2021

ldionne commented Jun 17, 2021

alexezeder left a comment

alexezeder Jun 17, 2021

ldionne Jun 17, 2021

alexezeder Jun 17, 2021

ldionne Jun 17, 2021

alexezeder Jun 17, 2021

coveralls commented Jul 10, 2021 •

edited

Loading

nlohmann left a comment

ldionne commented Jul 19, 2021

nlohmann left a comment

ldionne commented Jul 19, 2021

nlohmann commented Jul 19, 2021

ldionne commented Jul 19, 2021

nlohmann commented Jul 19, 2021

nlohmann commented Jul 22, 2021

alexezeder commented Jul 22, 2021

nlohmann left a comment

nlohmann commented Jul 22, 2021

		template<template<class...> class Op, class... Args>
		struct is_detected_lazy : is_detected<Op, Args...> { };

	detail::conjunction <
	std::integral_constant < bool,
	!std::is_pointer<ValueType>::value&&
	!std::is_same<ValueType, detail::json_ref<basic_json>>::value&&
	!std::is_same<ValueType, typename string_t::value_type>::value&&
	!detail::is_basic_json<ValueType>::value
	&& !std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value
	#if defined(JSON_HAS_CPP_17) && (defined(__GNUC__) \|\| (defined(_MSC_VER) && _MSC_VER >= 1910 && _MSC_VER <= 1914))
	&& !std::is_same<ValueType, typename std::string_view>::value
	#endif
	>,
	detail::is_detected<detail::get_template_function, const basic_json_t&, ValueType >>::value
	, int >::type = 0 >

Properly constrain the basic_json conversion operator #2825

Properly constrain the basic_json conversion operator #2825

Conversation

ldionne commented Jun 17, 2021

ldionne commented Jun 17, 2021

alexezeder left a comment

Choose a reason for hiding this comment

alexezeder Jun 17, 2021

Choose a reason for hiding this comment

ldionne Jun 17, 2021

Choose a reason for hiding this comment

alexezeder Jun 17, 2021

Choose a reason for hiding this comment

ldionne Jun 17, 2021

Choose a reason for hiding this comment

alexezeder Jun 17, 2021

Choose a reason for hiding this comment

coveralls commented Jul 10, 2021 • edited Loading

nlohmann left a comment

Choose a reason for hiding this comment

ldionne commented Jul 19, 2021

nlohmann left a comment

Choose a reason for hiding this comment

ldionne commented Jul 19, 2021

nlohmann commented Jul 19, 2021

ldionne commented Jul 19, 2021

nlohmann commented Jul 19, 2021

nlohmann commented Jul 22, 2021

alexezeder commented Jul 22, 2021

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented Jul 22, 2021

coveralls commented Jul 10, 2021 •

edited

Loading